Skip to content

fix: Fix Foldable1 NonEmptyArray instance (requires https://github.com/purescript/purescript-nonempty/pull/39) #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

srghma
Copy link
Contributor

@srghma srghma commented Oct 24, 2020

@srghma srghma mentioned this pull request Oct 24, 2020
@@ -1,6 +1,6 @@
"use strict";

exports.fold1Impl = function (f) {
exports.foldr1Impl = function (f) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the implementation for foldl1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I changed a name, because this name is better

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me clarify. The function name is foldr1Impl, but the implementation is what I would define for foldl1Impl. My issue isn't that the you changed the name. My issue is that the name refers to the wrong implementation. If you changed this to foldl1Impl and changed the currently named foldl1Impl to foldr1Impl, then the names would refer to the correct functions. You would also need to update the tests, since they are referring to the wrong outputs as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, fixed

assert $ foldr1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a"]) == "a"

log "foldl1 should work"
assert $ foldl1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a", "b", "c", "d"]) == "(a(b(cd)))"
Copy link
Contributor

@JordanMartinez JordanMartinez Oct 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this should be foldr1's test:

"( <> "a" <>
  "(" <> "b" <>
    "(" <> "c" <> "d" <> ")"
  <> ")"
<> ")"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, tnx, but the innermost d is not wrapped

the test is taken from https://github.com/purescript/purescript-nonempty/pull/39/files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I updated my code.

Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure you've swapped foldr1 and foldl1 implementations around.

@srghma
Copy link
Contributor Author

srghma commented Nov 7, 2020

Merged

@srghma srghma closed this Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants